Skip to content

Bound Cosmos diagnostics under retry storms#4683

Closed
NaluTripician wants to merge 33 commits into
Azure:mainfrom
NaluTripician:nalutripician/cosmos-diagnostics-storm-safe
Closed

Bound Cosmos diagnostics under retry storms#4683
NaluTripician wants to merge 33 commits into
Azure:mainfrom
NaluTripician:nalutripician/cosmos-diagnostics-storm-safe

Conversation

@NaluTripician

Copy link
Copy Markdown
Contributor

🔗 Stacked on #4619 (nalutripician/diagnostics-capture-redesign) — this change depends on #4619's DiagnosticsSummary and its re-homed diagnostics::capture model. Please review/merge this AFTER #4619 lands. Because #4619 is not in main yet, the diff below currently includes #4619's commits (~122 files); it will shrink to just these 7 files once #4619 merges into main:

  • sdk/cosmos/azure_data_cosmos_driver/src/diagnostics/capture/model.rs
  • sdk/cosmos/azure_data_cosmos_driver/src/diagnostics/capture/mod.rs
  • sdk/cosmos/azure_data_cosmos_driver/src/diagnostics/mod.rs
  • sdk/cosmos/azure_data_cosmos_driver/src/options/diagnostics_options.rs
  • sdk/cosmos/azure_data_cosmos_driver/tests/live_storm_diagnostics.rs
  • sdk/cosmos/azure_data_cosmos_driver/Cargo.toml
  • sdk/cosmos/azure_data_cosmos_driver/CHANGELOG.md

Make Cosmos diagnostics storm-safe: bounded-size compaction + live fault-injection validation. Follow-up to #4619.

Why

With the cheap Threshold gate, a latency/failure spike trips the gate for a large fraction of requests at once, so the expensive diagnostics materialization becomes steady-state exactly when the system is already stressed. And a single operation that retries a hot partition grows its per-attempt list — and its detailed JSON — without bound. This implements the diagnostics contract's bounded-size guarantee and empirically validates the concern.

Deliverable 1 — Retry-storm compaction (always-on model)

  • At operation finalization (DiagnosticsContextBuilder::complete), collapse runs of consecutive near-identical retries (same region / endpoint / status / sub-status / execution-context) into first + last + a count.
  • Bounded by a new configurable DiagnosticsOptions::max_request_diagnostics (with_max_request_diagnostics, env AZURE_COSMOS_DIAGNOSTICS_MAX_REQUESTS, default 512, min 16).
  • An order-robust global key-bucket fallback guarantees the retained count stays within the cap even under a region ping-pong (A→B→A→B), where consecutive run-length collapse alone would not.
  • Lossless for the signal that matters: the summary (added by [Prototype] Diagnostics capture engine (Cosmos driver) #4619) is computed from the full attempt list before compaction, so the (status, sub-status) histogram, retry/throttle counts, total RU and regions stay exact. request_count() still reports the true total; new retained_request_count() and compaction() (returning CompactionInfo / CompactedRun) expose the bounded count and per-run rollup.
  • Byte-identical when not triggered: detailed JSON gains a top-level compaction object only under a storm (skip_serializing_if). Regions stay normalized-lowercase. Additive and non-breaking.

Deliverable 2 — Threshold-storm validation

  • Deterministic in-crate measurement (storm_materialization_cost_and_size, #[ignore]d): a 100k-retry storm produces ~48 MB / 3.6 s of detailed JSON uncompacted vs 1.8 KB / 421 µs compacted (~27,000× smaller), while the summary still reports the exact retry count.
  • Env + feature-gated live test (tests/live_storm_diagnostics.rs, required-features = ["fault_injection"], #![cfg(feature = "reqwest")]) reads COSMOSDB_MULTI_REGION and injects latency + 429/503/410 storms, measuring gate-fire fraction and per-op materialization cost. Skips gracefully when the account is absent/unreachable; never prints secrets — so CI/playback never run it.
  • Mitigations recommended back to the contract: structural compaction (this PR), configurable diagnostics depth under stress, sampling + max-events-per-interval cap, lazy/handle materialization.

Guardrails

  • capture_engine untouched / still off by default; no Off mode; CosmosResponse::diagnostics() stays non-optional.
  • Live tests are env + feature gated; no secrets committed.
  • CHANGELOG updated for the additive public API (D1); D2 is internal tests only.

Testing

  • cargo fmt clean; cargo clippy --all-features --all-targets -- -D warnings clean.
  • cargo test -p azure_data_cosmos_driver --all-features: 1999 lib + all integration tests pass (incl. new compaction unit tests, options tests, and the live storm test against the real account).

NaluTripician and others added 24 commits June 15, 2026 13:03
Adds a parallel, opt-in diagnostics CAPTURE subsystem to the Cosmos driver
under `diagnostics::capture`, coexisting with the existing DiagnosticsContext
(which is left untouched). It productionizes a benchmarked "Deferred Gated
Capture" design: a compact, append-only, lock-free hot path that builds an
aggregatable summary (and opt-in AZD1 binary detail) only when an op-end gate
decides it is worth it.

Subsystem (src/diagnostics/capture/): wire (AZD1 codec, reject-unknown-version
+ hardened decode), bounded LogPool, lock-free Drop/panic-safe
DiagnosticsRecorder (+ fan-out ChildRecord/merge_child), signals-aligned
Summary, gate (Off/Threshold/Always), interned version/User-Agent preamble.

Additive wiring (default Off => no behavior change, no recorder constructed):
- diagnostics/mod.rs: `pub mod capture;` (existing exports preserved).
- DriverOptions::with_capture_diagnostics_policy / capture_diagnostics_policy.
- CosmosDriver::execute_operation_direct records the operation-level outcome
  when enabled and attaches it via CosmosResponse::capture_diagnostics().
- lib.rs re-exports (Capture* aliases); criterion dev-dep + bench; flate2 dep.

Parallel by design: it currently builds its own Summary/Rendered. Feeding or
extending DiagnosticsContext instead is the deferred open question, documented
in DIAGNOSTICS-CAPTURE.md. No azure_core/typespec_client_core changes.

Bench (release): dropped fast-success ~140 ns; built summary ~1.6 us; built
detailed ~54 us. fmt + clippy --all-features --all-targets -D warnings +
cargo test --all-features all pass (1940 lib tests incl. 28 capture).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Make the deferred, threshold-gated capture front-end build the canonical
`DiagnosticsContext` instead of a parallel `Summary`/`Rendered`/`AZD1`
model. There is now one diagnostics model: the cheap, append-only, lock-free
hot-path recorder plus the op-end gate, and past the gate the captured log is
replayed onto the existing `DiagnosticsContextBuilder`.

- recorder.rs: new `AttemptRecord` value type (exec-context, region,
  endpoint, status, sub-status, service-request-id, RU, request_sent,
  durations) + `record_hedge_outcome` + new `record_end` signature; inlined
  the varint/TLV codec (wire.rs removed).
- context.rs (new): `build_context` maps captured attempts to
  `RequestDiagnostics` and attaches `HedgeDiagnostics` (region legs,
  winning leg, terminal state) for hedged operations.
- gate.rs: `finish` returns `Option<DiagnosticsContext>`; added an
  `off()` constructor for symmetry.
- Retired the parallel `Summary`/`Rendered`/`AZD1` wire format and the
  interned version preamble; dropped the now-unused `flate2` dependency.
- cosmos_response.rs: `capture_diagnostics()` returns `&DiagnosticsContext`.
- cosmos_driver.rs: op-executor wiring builds and attaches the context; the
  error path logs the built context's JSON.
- Added `tests/diagnostics_examples.rs` (real normal + hedged
  `DiagnosticsContext` JSON), an `Off`-mode criterion bench row, and
  updated DIAGNOSTICS-CAPTURE.md + CHANGELOG to the integrated framing.

Full gate green on upstream/main: fmt --check, clippy --all-features
--all-targets -D warnings, cargo test --all-features. No azure_core /
typespec_client_core changes; capture stays opt-in (default Off).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Make the gated-capture module the driver's diagnostics ENGINE and the OWNER
of the canonical diagnostics model, without losing rich data or breaking the
public SDK boundary. This is an ownership flip scoped to the driver crate.

- Re-home the rich model: move `diagnostics/diagnostics_context.rs` to
  `diagnostics/capture/model.rs`. `capture` now owns `DiagnosticsContext`,
  `DiagnosticsContextBuilder`, `RequestDiagnostics`, `ExecutionContext`,
  the transport-shard / fault-injection / event types, etc. `diagnostics/mod.rs`
  re-exports them from `capture` so the public paths
  (`diagnostics::DiagnosticsContext`, consumed by `azure_data_cosmos`) are
  unchanged. The pipeline keeps feeding the (now capture-owned) builder, so
  every rich field + true wall-clock timing is preserved — no data loss.
- Default the gate to `Always` (was `Off`): diagnostics are produced
  out-of-the-box, matching the driver's historical always-on behavior;
  configurable to `Threshold`/`Off` via `DriverOptions`.
- Route through the gate: at the operation-executor seam the recorder records
  the outcome + elapsed and the gate decides whether the canonical
  `DiagnosticsContext` is surfaced via `CosmosResponse::capture_diagnostics()`
  — one model, gated, not rebuilt as a parallel object. The error path logs the
  error's canonical diagnostics when the gate fires.
- Docs: rewrite `DIAGNOSTICS-CAPTURE.md` + the CHANGELOG to the ownership-flip
  framing (additive / non-breaking to the public SDK; default Always).

Public boundary verified intact: `azure_data_cosmos` builds + its 92 lib
tests pass; `response.diagnostics()` and the public `DiagnosticsContext` type
are unchanged. Full gate green on the driver crate: fmt --check, clippy
--all-features --all-targets -D warnings, cargo test --all-features (1926 lib +
2 examples + doctests + 45 integration). No azure_core / typespec changes.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Wire the capture gate so it decides BEFORE the rich build happens, instead of
building then dropping. Adds an `enabled` flag to `DiagnosticsContextBuilder`:
when the capture policy is `Off` the builder is created disabled, so per-request
population is genuinely skipped on the hot path.

- model.rs: `DiagnosticsContextBuilder` gains `enabled` + `new_disabled()`.
  A disabled `start_request` returns an out-of-range `RequestHandle(usize::MAX)`
  without pushing, so every handle method (`update_request` / `complete_request`
  / `add_event` / transport-shard / fault-injection) auto-no-ops via the existing
  `requests.get_mut(handle.0)` miss — no changes to those call sites.
  `set_hedge_diagnostics` + `merge_hedge_attempt` guard against re-population;
  `clone_for_hedge_attempt` propagates `enabled`. `complete()` then yields a
  minimal context (activity id + final status, empty requests).
- cosmos_driver.rs: `new_diagnostics_envelope` takes `enabled` and builds a
  disabled builder when `capture_policy.mode == Off` (skipping the CPU-monitor /
  machine-id / fault-injection setup); the bootstrap account-fetch stays enabled.
- gate.rs: drop `Default` from `Mode` (skeptic-review fix) so a stray
  `Mode::default()` can't silently return `Off` against the `Always` policy
  default; the meaningful default lives on `DiagnosticsPolicy`.
- Tests: disabled builder records nothing + handle methods are safe no-ops;
  enabled builder still records fully (guards the default Always path).
- Docs: `DIAGNOSTICS-CAPTURE.md` documents the Off short-circuit and the precise
  `Threshold` fast-success boundary (rich data is incremental; not droppable
  without a full collection rewrite — no data loss when the gate fires); CHANGELOG
  updated.

Boundary (reported, not silently dropped): `Threshold` fast-success cannot avoid
the pipeline's incremental build without data loss, because the slow/error verdict
is only known at op-end and the always-on `response.diagnostics()` needs the
context; that build is cheap (move + Arc, lazy JSON) and the standalone
materialization is already gated.

Adversarial skeptic review: all 8 invariants verified (no data loss when the gate
fires, handle-safe, no control-flow dependency, hedging-safe, panic/Drop-safe,
default Always unchanged, public boundary intact, no secrets); the one finding
(Mode Default footgun) is fixed. Full gate green: fmt --check, clippy
--all-features --all-targets -D warnings, cargo test --all-features (1928 lib + 2
examples + 45 integration). Public `azure_data_cosmos` builds + 92 lib tests
pass. No azure_core / typespec changes.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Record the conclusion of the full collection-through-append-log investigation
in DIAGNOSTICS-CAPTURE.md §8: it is not a net win and is rejected. Capture the
three blockers concisely (full fidelity != sub-us; the builder is already
lock-free append + lazy JSON with Off disabling per-request population; Instant
timestamps and the non_exhaustive FaultInjectionEvaluation enum can't be
losslessly byte-encoded) and state the resolution — keep the current design
(full fidelity, lock-free append, lazy JSON, Off as the cheap opt-out, default
Always). Doc-only; no code changes.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Two additive, non-breaking diagnostics features on the driver crate.

FEATURE 1 — .NET-style top-level Summary block:
- Add DiagnosticsSummary (SafeDebug, Serialize) to DiagnosticsContext,
  reachable via DiagnosticsContext::summary(). Modeled on .NET
  CosmosDiagnostics' top-level Summary: a (status, sub-status) histogram, total
  request charge, request/retry/throttle counts, regions contacted, final
  status, top error, total elapsed.
- Computed ONCE at FINALIZATION (in complete() and �ggregate_sub_operations)
  as a reduction over the already-collected requests — never on the hot path.
  It therefore exists only when a context is built; a dropped fast success
  produces no context and no summary.
- Serialized as the first top-level section of the diagnostics output (like
  .NET puts Summary at the top). Parity tests for typical / retry+throttle /
  error / hedged. Existing structural golden tests strip the summary key (it has
  dedicated tests + timing-dependent total_duration_ms).

FEATURE 2 — diagnostics encoding as a client option:
- Add DiagnosticsEncoding { Json (pretty, default), Compact (minified),
  Encoded (base64 of compact JSON) } and expose it on DriverOptions
  (with_diagnostics_encoding / diagnostics_encoding).
- Add DiagnosticsContext::encode(encoding) -> String and
  CosmosResponse::diagnostics_string(encoding) (pass the configured option).
  Default Json keeps existing output unchanged; Encoded round-trips via base64.

Docs (DIAGNOSTICS-CAPTURE.md §5a/§5b) + CHANGELOG + examples test updated. Full
gate green: fmt --check, clippy --all-features --all-targets -D warnings, cargo
test --all-features (1934 lib + 3 examples + 45 integration). Public
azure_data_cosmos builds + 92 lib tests pass (boundary unchanged). No
azure_core / typespec changes.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add serializable absolute timestamps alongside the existing
`Instant`-based durations so diagnostics can be correlated against
server-side and external logs.

- `DiagnosticsContext` gains an operation `start_time`/`end_time`
  (captured at recorder creation and at `complete()`), with matching
  accessors. The `DiagnosticsSummary` carries the same window.
- Each `RequestDiagnostics` attempt gains a `start_time` and an optional
  `end_time` (captured on complete/timeout/transport-failure, omitted
  via `skip_serializing_if` while in flight).
- Captured with `azure_core::time::OffsetDateTime` and serialized as
  RFC 3339 via a small `rfc3339` serde helper — no new dependency and no
  `azure_core` change.

Additive and non-breaking: the fields exist only when a context is built,
durations are unchanged, and the default output is otherwise the same.
Golden JSON tests strip the non-deterministic timestamps in
`normalize_diagnostics_json`; a new `timestamps_present_and_rfc3339` test
plus the typical-operation parity test assert presence and RFC 3339
format. Updated DIAGNOSTICS-CAPTURE.md (§5c) and the CHANGELOG.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace the diagnostics-capture recorder's Vec<u8> TLV byte stream with a typed, two-list event log (Vec<Span> + Vec<Attr>). Recording an event is a single Vec::push of a small typed value -- no varints or byte encoding on the hot path. A span carries a kind, an optional parent id, and op-relative timestamps (its id is its index); attributes are typed key/value pairs tagged with their owning span.

- event.rs (new): SpanKind, Span, AttrKey, AttrValue, Attr, EventLog with push/clear/reconstruction helpers and a public compact-bytes round-trip.
- pool.rs: pool the two-Vec EventLog instead of Vec<u8>; bounded retention.
- recorder.rs: DiagnosticsRecorder pushes typed spans/attrs; same public API (AttemptRecord/HedgeOutcome/start/record_*); Drop/panic-safe pooling.
- context.rs: walk the spans/attrs tree onto DiagnosticsContextBuilder -- the typed log is the parsed form, so the byte-parse step is gone.
- encode.rs (new): optional cold-path varint/TLV serialization of the two lists, with round-trip + truncation tests.
- gate.rs: finish() builds from the EventLog then returns it to the pool.
- mod.rs: module decls + event-type re-exports.
- DIAGNOSTICS-CAPTURE.md (5d) + CHANGELOG + bench doc updated.

Internal to the capture engine; the public DiagnosticsContext output and the azure_data_cosmos boundary are unchanged. fmt + clippy --all-features --all-targets -D warnings clean; capture unit tests, diagnostics_examples, full lib suite (1944), and doctests pass.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…to nalutripician/diagnostics-capture-redesign

# Conflicts:
#	Cargo.lock
#	sdk/cosmos/azure_data_cosmos_driver/CHANGELOG.md
#	sdk/cosmos/azure_data_cosmos_driver/src/driver/cosmos_driver.rs
#	sdk/cosmos/azure_data_cosmos_driver/src/options/driver_options.rs
After merging main, the live capture diagnostics test used the removed runtime.get_or_create_driver(account, Some(options)) entry point. Update it to create_driver(driver_options), which now carries the account via DriverOptions::builder(account).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…to nalutripician/diagnostics-capture-redesign

# Conflicts:
#	sdk/cosmos/azure_data_cosmos_driver/src/diagnostics/capture/model.rs
cSpell flagged 'verbosities' in diagnostics/capture/model.rs, failing the Analyze job's spell-check step.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Addresses Ashley Stanton-Nurse's design review on PR Azure#4619:

- EventLog is now an RAII lease (Arc<LogPool> + inner EventLogStorage); dropping it returns the storage to the pool automatically. Removes the explicit return_buffer step and the recorder's Drop/Option plumbing. (#9, #10)
- LogPool holds a plain Mutex<Vec<EventLogStorage>> instead of an internal Arc; consumers hold Arc<LogPool> and rent() takes &Arc<Self>, so sharing is explicit at the call site. (#11)
- Removed LogPool::new(); callers use LogPool::default(). (#2)
- Default storage capacity (DEFAULT_SPANS=8, DEFAULT_ATTRS=32) sized for the common 1-4-attempt op; the pool only retains storages still at the default capacity and frees any that grew. (#1, #3)
- SpanId(NonZeroU32) newtype (index + 1), with Option<SpanId> parents replacing the NO_PARENT sentinel; the niche makes Option<SpanId> the size of a u32. (#4)
- AttrValue gains StaticStr(&'static str), SharedStr(Arc<str>), and a first-class Status(CosmosStatus) variant alongside U64/F64/Str, so the hot path can store regions/statuses without boxing. (#5, #6, #7)
- TimeOffset newtype for span start/end instead of _ns-suffixed u64 fields. (#8)

The cold-path encode/decode, context reconstruction, recorder, driver wiring, bench, and tests are updated to the new types. Public DiagnosticsContext output and the azure_data_cosmos boundary are unchanged. fmt + clippy --all-features --all-targets -D warnings, cargo doc -Dwarnings, cspell, capture unit tests (80), diagnostics_examples, full lib suite (1965), and the capture doctest all pass.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
DiagnosticsSummary now carries the client User-Agent (SDK + runtime identity), surfaced via DiagnosticsSummary::user_agent() and serialized inside the top-level summary block. The DiagnosticsContextBuilder gains a user_agent field + set_user_agent setter (propagated across hedge-attempt clones), and the driver's new_diagnostics_envelope feeds it from the runtime's user agent. It is omitted from the JSON when not supplied, so existing output is unchanged.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Every pipeline event in a request's `events` list was serialized with
`"duration_ms": null`. Events are recorded via `RequestEvent::new`, which
leaves `duration_ms` as `None`, and the only constructor that sets it
(`with_duration`) is used solely in tests -- so no production path ever
populated the field and it was always null.

Stamp the stage duration when an event is recorded on a request:
`RequestDiagnostics::add_event` now fills `duration_ms` (when the caller
did not supply one) with the time elapsed since the previous event, or
since the request started for the first event. So `response_headers_received`
carries the transport time-to-first-byte and `transport_failed` carries the
time until the failure. A caller-provided duration is still respected.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Gate the unfinished event-log capture engine behind an off-by-default
`capture_engine` feature and align all docs/claims with what actually
ships, with no fidelity regression on the default path.

- A1: feature-gate event/context/encode/pool/recorder + gate::finish and
  their re-exports behind `capture_engine` (default OFF). Keep the gate
  (Mode/DiagnosticsPolicy/should_build) and the canonical model
  unconditional. Gate the engine-only example test + bench on the feature.
- A3/A1: the driver no longer rents the inert recorder. It computes the
  exposure gate directly (outcome + elapsed) so `capture_diagnostics()`
  behavior is unchanged with the feature off; the builder remains the sole
  populator of the surfaced context.
- A4: drop per-op `endpoint.to_string()` / `format!("{op:?} {res:?}")`.
- A2: compute `DiagnosticsSummary` lazily via `OnceLock` on first access
  instead of eagerly in `complete()`; JSON output stays byte-identical.
- A6: add symmetric `CosmosError::capture_diagnostics()` populated by the
  same gate; replace the tracing::debug!-only failure handling.
- A5: hardcoded attempt-count `1` removed with the inert wiring; true-count
  plumbing belongs to the flagged B-series when capture is wired.
- A9: rewrite DIAGNOSTICS-CAPTURE.md, CHANGELOG, capture/driver_options
  rustdoc to the honest state (default path = builder; capture behind the
  flag; reconstruction still lossy; parity gate pending).

build/test/clippy/fmt green with and without `capture_engine`.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Make the capture-engine reconstruction faithful instead of fabricating,
add a parity harness and concurrency/codec hardening tests, and correct
the docs accordingly. All behind the off-by-default capture_engine feature
(exercised by CI via --all-features); the default diagnostics path is
unchanged.

- Capture per-attempt pipeline type, transport security, transport kind,
  HTTP version, and the server-reported duration into the event log
  (new AttrKeys + AttemptRecord::with_transport / with_server_duration_ms),
  and reconstruct them in context.rs instead of hardcoding
  DataPlane/Secure/Gateway/Http2 and the client-observed span.
- Add a parity harness asserting the reconstruction carries the real
  facets and matches a builder-built reference field-by-field.
- Cap decode() pre-allocation against the remaining input so a malformed
  header cannot drive a giant allocation; add a regression test.
- Add a multi-threaded LogPool rent/return stress test.
- Refresh DIAGNOSTICS-CAPTURE.md, CHANGELOG, and module docs to the
  current state (facets captured + parity-checked; remaining work is
  feeding the recorder from the live pipeline).

build/test/clippy/doc/fmt green with and without capture_engine.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The driver evaluates the exposure gate directly; it no longer rents the
event-log recorder on the default path. Fix the scope section accordingly
and drop a dangling cross-reference.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The capture_engine feature gates the DiagnosticsContext import in gate.rs,
so the bare intra-doc link on DiagnosticsPolicy::always broke under the
docs.rs feature set (which excludes capture_engine), failing the CI
'cargo +nightly docs-rs' step. Use the fully-qualified link path so it
resolves regardless of the imported-into-scope items.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The Build Analyze job runs cSpell, which flagged three coined abbreviations
in test/comment identifiers. Rename to dictionary words:
- parity test local refr -> ref_req
- encode test name overallocate -> over_allocate
- model.rs Clone comment uncomputed -> unset

Verified clean with eng/common/spelling/Invoke-Cspell.ps1 over all changed
files; capture tests still pass.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
cargo fmt wraps the transport_http_version assertion now that the local
rename pushed it past the 100-col width. Verified the full Analyze gate
locally (fmt, taplo, check, clippy, doc, cargo +nightly docs-rs, cSpell).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Section A items 1 & 2 from the diagnostics-sync review with Fabian.

Item 2 - remove the `Off` diagnostics-capture policy entirely so diagnostics
are always collected (a customer who silently dropped diagnostics is
unsupportable). Default stays `Always`; the gate only governs *exposure*:
- gate.rs: drop `Mode::Off`, `DiagnosticsPolicy::off()`, the `Mode::Off` arm in
  `should_build`, and the `off_never_builds` test.
- model.rs: drop the `DiagnosticsContextBuilder` `enabled` flag, `new_disabled()`,
  `is_enabled()`, and the `if !enabled` guards in `start_request` /
  `set_hedge_diagnostics` / `merge_hedge_attempt`; the builder always records.
- cosmos_driver.rs: `new_diagnostics_envelope` no longer takes/branches on
  `enabled`; drop the `diagnostics_enabled` computation and the
  `if diagnostics_enabled` wrappers in the op-end gate (success + error paths).
- Update Off references in driver_options.rs, cosmos_response.rs, capture/mod.rs,
  DIAGNOSTICS-CAPTURE.md, and the unreleased CHANGELOG entry.

Item 1 - clarify what the capture benchmark measures. `capture_built_context`
builds the `DiagnosticsContext` struct only (no JSON). Added
`capture_built_context_and_json` which also serializes the detailed JSON, so the
two isolate the serialization cost. Measured (criterion, indicative): gate-drop
~0.86us, struct-build ~3.7us, struct+JSON ~10.4us - JSON serialization adds
~6.6us and is the single most expensive step, supporting keeping serialization
on-demand/cached in the SDK. Updated the section 3 table and section 7 run
instructions.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add always-on retry-storm compaction to the DiagnosticsContext model plus
live fault-injection validation of threshold-gate materialization cost.
Follow-up to PR Azure#4619.

Deliverable 1 (compaction): at operation finalization, collapse runs of
consecutive near-identical retries (same region/endpoint/status/sub-status/
execution-context) into first + last + a count, bounded by a new
configurable DiagnosticsOptions::max_request_diagnostics cap (env
AZURE_COSMOS_DIAGNOSTICS_MAX_REQUESTS, default 512, min 16). An order-robust
global key-bucket fallback keeps the retained count within the cap even under
a region ping-pong. The summary is computed from the full attempt list before
compaction, so the (status, sub-status) histogram, retry/throttle counts,
total RU and regions stay exact; request_count() still reports the true total,
with new retained_request_count() and compaction() accessors (CompactionInfo /
CompactedRun). Detailed JSON gains a skip-if-absent compaction marker, so
normal-operation output is byte-identical. Additive and non-breaking.

Deliverable 2 (validation): a deterministic in-crate measurement quantifies
the bound (100k-retry storm: ~48 MB / 3.6 s detailed JSON uncompacted vs
1.8 KB / 421 us compacted) and an env+feature-gated live test
(tests/live_storm_diagnostics.rs, reading COSMOSDB_MULTI_REGION) corroborates
the gate-fire behavior and per-op materialization cost with fault injection.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR makes Cosmos driver diagnostics storm-safe by adding bounded-size compaction to the always-on DiagnosticsContext and a live fault-injection validation test. Under a 4xx/5xx retry storm a single operation could otherwise grow an unbounded per-attempt list (and unbounded detailed JSON). Compaction collapses runs of near-identical retries into first + last + a count at operation finalization, bounded by a new configurable DiagnosticsOptions::max_request_diagnostics (default 512, min 16, env AZURE_COSMOS_DIAGNOSTICS_MAX_REQUESTS), while the pre-compaction summary, request_count(), and total RU stay exact. Note: this PR is stacked on the unmerged #4619, so the diff currently also contains #4619's diagnostics-capture redesign (the diagnostics::capture module, gate wiring, encoding option, etc.); the standalone delta is the compaction and the live test.

Changes:

  • Add retry-storm compaction (in model.rs, not shown in this diff) plus new public accessors retained_request_count() / compaction() and CompactionInfo/CompactedRun, re-exported from diagnostics and diagnostics::capture.
  • Add DiagnosticsOptions::max_request_diagnostics with builder/env parsing, validation, and unit tests.
  • Add an env + feature-gated live storm validation test and its Cargo target; document the additive API in the CHANGELOG.

Reviewed changes

Copilot reviewed 24 out of 25 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/options/diagnostics_options.rs Adds max_request_diagnostics (default 512/min 16) + env parsing/tests; also #4619's DiagnosticsEncoding
tests/live_storm_diagnostics.rs New env+feature-gated live storm test measuring gate-fire and compaction bounds
src/diagnostics/mod.rs Re-exports CompactedRun/CompactionInfo (plus #4619 capture module re-homing)
src/diagnostics/capture/mod.rs Re-exports compaction types from model; #4619 capture module root
CHANGELOG.md Adds the compaction feature entry under "Features Added"
Cargo.toml Registers the live_storm_diagnostics test target + #4619 bench/criterion dev-dep
sdk/cosmos/.cspell.json Adds spelling dictionary terms (aggregatable, varint(s), Vecs, verbosities)
src/models/cosmos_response.rs, src/error/mod.rs, src/driver/cosmos_driver.rs, src/lib.rs, src/options/{mod,driver_options}.rs, src/diagnostics/capture/{recorder,pool,gate,event,encode,context}.rs, benches/diagnostics_capture.rs, tests/{live_capture_diagnostics,diagnostics_examples}.rs, DIAGNOSTICS-CAPTURE.md, Cargo.lock Carried in from the stacked #4619 (capture engine, gate, encoding, docs); reviewed under #4619

Comment thread sdk/cosmos/azure_data_cosmos_driver/tests/live_storm_diagnostics.rs Outdated
@NaluTripician

Copy link
Copy Markdown
Contributor Author

Reviewed this together with the contract doc (#4684) and the OTel spike (#4685). Overall the compaction design is solid — output stays byte-identical when compaction doesn't fire, the aggregate summary is computed before the lossy step so the histogram/counts stay exact, and the global-bucket fallback is deterministic (first-seen order, no HashMap iteration nondeterminism). A few things I'd want addressed before this merges:

Blocking — the bounded-size guarantee is only partial

global_bucket_compact truncates retained to cap, but never bounds runs:

if retained.len() > cap { retained.truncate(cap); }   // runs: Vec<CompactedRun> is left at one-per-distinct-key

CompactionInfo.runs is serialized into every detailed/summary output, and its length is the number of distinct CompactionKeys (region / endpoint / status / sub-status / execution-context). A 410/Gone fan-out across many physical-partition endpoints is high-cardinality exactly when the storm is worst, so the detailed JSON grows O(distinct keys) — bounded by topology, not by max_request_diagnostics. The two "many" tests don't catch this: region_ping_pong… uses 2 keys, and retry_storm_is_bounded_and_lossless asserts the 16 KB bound with a single key.

Suggest bounding runs too (top-N runs + an "other" rollup) and adding a distinct-key JSON-size test (e.g. 5000 unique endpoints → assert json.len() stays bounded).

Silent truncation contradicts the contract

The contract (#4684 §6/Q4) says "Truncation is marked, never silent" and "first+last-per-region always retained." The retained.truncate(cap) above silently drops later buckets with no marker in the retained list. Either surface a marker or amend the doc.

Eager summary on the completion path

When original_count > cap, complete() now computes the summary eagerly for every stormed op even when diagnostics are never read — this regresses the lazy design the (removed) comment described, and it fires precisely when the system is already under load. The #[ignore]d bench always reads the JSON, so it never measures the never-inspected case.

Peak memory mid-storm is still unbounded

Compaction runs only at complete(); self.requests grows one entry per attempt while the operation is in-flight. Worth stating explicitly that the guarantee is scoped to the final serialized artifact, not live memory.

request_count() semantic change

It now returns the pre-compaction total, so request_count() != requests().len() after a storm. Please audit azure_data_cosmos callers that size or iterate over requests() using request_count(). (The #4685 spike encodes exactly this pattern — see the comment there.)

Live test hardcodes the write region

error_rule(…, Region::new("West US 2"), …) while the account is supplied via COSMOSDB_MULTI_REGION. If the write region differs (the standard multi-region test account uses East US 2 / West US 3), the error rules never match, no storm is induced, compacted_ops stays 0, and the test passes green without exercising the compaction it exists to validate. Derive the region from a first probe's regions_contacted() (or document the account assumption).

Nit: the live test's std::env::set_var("AZURE_COSMOS_DIAGNOSTICS_MAX_REQUESTS", …) is process-global (and set_var is unsound under concurrency in newer Rust) — prefer setting the cap through the builder, or serialize the test.

NaluTripician added a commit to NaluTripician/azure-sdk-for-rust that referenced this pull request Jul 6, 2026
Address self-review on PR Azure#4685:

- Count/iterate attempt spans off ctx.requests() (the retained list the
  spans are actually built from) instead of ctx.request_count(). On main the
  two are equal, but once PR Azure#4683's retry-storm compaction lands
  request_count() reports the true pre-compaction total while requests() is
  the bounded first+last-per-run list, which would break the assertions. A
  faithful emitter must count off the list it emits from.
- Note (DIAGNOSTICS-CONTRACT.md 7.3) that under compaction a run collapses to
  a single span + count, so an emitter cannot produce request_count() spans.
- Document the otel_spans_spike feature dep-scoping caveat: it enables
  opentelemetry(+_sdk) unconditionally though the only consumer is the
  test-gated module, so a non-test --all-features build pulls them in unused.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
NaluTripician added a commit to NaluTripician/azure-sdk-for-rust that referenced this pull request Jul 6, 2026
Address self-review on PR Azure#4684:

- Reconcile 6/Q4 bounded-size numbers with what PR Azure#4683 actually ships:
  the per-attempt list is bounded by max_request_diagnostics (default 512,
  min 16), keyed on (region, endpoint, status, sub_status, execution_context),
  first+last-per-run + exact aggregates + a bounded per-run rollup. The old
  64-attempt / 128-span / 8 KB figures are re-labelled as target defaults for
  the not-yet-implemented span/string representations and the deferred engine,
  and FFI buffer sizing (4 rule 5) is told to read the configured cap.
- Clarify deferred/OFF vs hard dependency: the contract doc is standalone on
  main and depends on neither Azure#4619 nor Azure#4683, but the Azure#4683 compaction impl
  reuses Azure#4619's DiagnosticsSummary and must merge after it -- so the
  deferred/OFF language describes the capture engine, not the compaction impl.
- Note in 7.3 that the one-span-per-attempt mapping is the uncompacted view;
  under compaction a run collapses to one span + a repeat count, so emitters
  build the tree from requests() (retained), not request_count().

Documentation only; markdownlint clean.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
NaluTripician and others added 2 commits July 6, 2026 10:56
…s-diagnostics-storm-safe

# Conflicts:
#	Cargo.lock
#	sdk/cosmos/azure_data_cosmos_driver/src/options/driver_options.rs
Address self-review on PR Azure#4683:

- Bound the per-run rollup, not just the retained list (BLOCKING). CompactionInfo.runs
  was O(distinct keys), so a high-cardinality 410 fan-out grew the detailed JSON by
  topology. bound_runs() now keeps the largest runs by attempt count (deterministic
  tie-break) up to the cap and rolls the remainder into explicit omitted_runs /
  omitted_request_count markers. New distinct_key_fanout_runs_and_json_are_bounded test
  (5000 unique endpoints) asserts the JSON stays cap-bounded and lossless.
- Surface truncation explicitly (never silent). CompactionInfo gains total_runs and
  retained_truncated; the global-bucket retained.truncate(cap) now sets the marker.
- Eager summary: document that the storm path must aggregate eagerly (compaction drops
  the source records, so the exact summary cannot be recovered lazily); the fast-success
  path stays lazy. Extend the deterministic bench to measure the never-inspected
  complete() cost (ON eager-summary vs OFF lazy).
- Peak memory: document that the bound is on the finalized serialized artifact + retained
  list, not live mid-operation memory.
- request_count() audit: SDK callers use lower-bound checks / iterate requests() directly
  and stay correct under compaction; fixed the one cosmetic denominator in
  assert_region_not_contacted to report retained requests.len().
- Live test: derive the fault-injection region from the baseline probe's
  regions_contacted() instead of a hardcoded region (a mismatch would silently induce no
  storm and pass green); skip gracefully if none. Justify the single process-global env
  write (one test per binary => serialized; cap is env-only at the driver today).

Also resolves the earlier merge with upstream/main (diagnostics_options.rs now follows
upstream from_env + resolve_from_env pattern for max_request_diagnostics).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@NaluTripician

Copy link
Copy Markdown
Contributor Author

Addressed in 085f87f (plus a merge of upstream/main that resolves the conflict — the PR is now MERGEABLE):

Blocking — bounded-size guarantee was only partial. CompactionInfo.runs was O(distinct keys), so a high-cardinality 410 fan-out grew the detailed JSON by topology. runs is now bounded by bound_runs() (keep the largest runs by attempt count, deterministic tie-break by first-seen order, up to the cap) with the remainder rolled into explicit omitted_runs / omitted_request_count. Added distinct_key_fanout_runs_and_json_are_bounded (5000 unique endpoints) asserting the detailed JSON stays cap-bounded and lossless.

Silent truncation. CompactionInfo now carries total_runs and retained_truncated; the global-bucket retained.truncate(cap) sets the flag, so both retained-list truncation and run omission are explicit, never silent. Reconciled with the contract doc (#4684 §6/Q4).

Eager summary. Documented in complete() that the storm path must aggregate eagerly: compaction drops the source records, so the exact summary can't be recovered lazily, and retaining the full list to stay lazy would defeat the bounded-size guarantee. The fast-success (≤ cap) path stays fully lazy. Extended the deterministic bench to also measure the never-inspected complete() cost (ON eager-summary vs OFF lazy), addressing the "bench never measures the never-inspected case" note.

Peak mid-storm memory. Documented (code + CHANGELOG) that the guarantee is scoped to the finalized serialized artifact + retained list, not live mid-operation memory.

request_count() semantic change. Audited the azure_data_cosmos callers: they all use lower-bound checks (> 1, >= 2, >= 1) or iterate requests() directly, so they stay correct under compaction (first+last are always retained). Fixed the one cosmetic mismatch — assert_region_not_contacted now reports requests.len() (retained) as the denominator instead of request_count().

Live test hardcoded write region. Now derived from the baseline probe's regions_contacted() (skips gracefully if none), so a region mismatch can't silently induce no storm and pass green.

set_var nit. The cap is env-only at the driver today (DiagnosticsOptions::default() resolves AZURE_COSMOS_DIAGNOSTICS_MAX_REQUESTS per operation; there's no per-driver builder override yet), so the env is the only channel. Documented that this binary has exactly one #[tokio::test], so the write is serialized — and flagged a per-driver with_max_request_diagnostics knob as the preferred follow-up.

Merge conflict. Merged upstream/main; the only real conflicts were Cargo.lock (regenerated) and a driver_options.rs test block (kept both sides). Reconciled diagnostics_options.rs onto upstream's from_env + resolve_from_env pattern for max_request_diagnostics.

cargo fmt + cargo clippy --features fault_injection --all-targets clean; 1983 driver lib tests pass (incl. the new + existing compaction tests); the azure_data_cosmos test build is clean.

… storm test

Address review nits on retry-storm diagnostics compaction:

- Coherence: under the Phase-2 global key-bucket fallback, the retained
  per-attempt records and the per-run rollup are now drawn from ONE
  ranking (the cap-largest runs by attempt count), so every retained
  record has a matching run in the rollup. Previously retained records
  were the earliest first-seen buckets while the rollup kept the
  highest-count buckets, so under heterogeneous run counts a retained
  attempt could belong to a run omitted from the rollup - breaking the
  driver->SDK span-emitter mapping in DIAGNOSTICS-CONTRACT.md 7.3. Folded
  the separate bound_runs step into global_bucket_compact and surfaced
  total_runs/omitted_runs/omitted_request_count via CompactionResult.
  Added phase2_heterogeneous_runs_keep_largest_and_stay_coherent, which
  fails on the old selection and passes now.

- live_storm_diagnostics: replace the weak max_request_count >= 1 check
  with a loud guard - when the storm batch reaches the service it must
  induce retries beyond the fault-free baseline, so a region-scoped fault
  that silently fails to match (region-name normalization mismatch) now
  fails the test instead of passing green without exercising compaction.

cargo fmt + cargo clippy --all-features --all-targets clean; all model
compaction unit tests pass.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
NaluTripician added a commit to NaluTripician/azure-sdk-for-rust that referenced this pull request Jul 6, 2026
…ntract

Address review nit: clarify that the shipped 512 structured-object record
cap and the 128-span target are independent per-representation caps, not a
single number. A span emitter reaches <=128 spans by collapsing each run to
one span + a repeat count (per 7.3), so span count tracks runs (already
capped), not retained records. Also note PR Azure#4683's guarantee that the
retained attempts and the per-run rollup are drawn from the same cap-bounded
set, so an emitter never sees a retained attempt whose run was omitted.

Documentation only.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@NaluTripician

Copy link
Copy Markdown
Contributor Author

Addressed the two round-2 review nits in fa3ff2804:

1. Phase-2 record/rollup coherence (the substantive one). Under the global key-bucket fallback, the retained per-attempt records and the per-run rollup were selected by different criteria — retained kept the earliest first-seen buckets, while the rollup kept the highest-count buckets. Under heterogeneous run counts those sets diverge, so a retained attempt could belong to a run omitted from the rollup, breaking the §7.3 span-emitter mapping. Fixed by folding the separate bound_runs step into global_bucket_compact and driving both the retained records and the rollup from one ranking (the cap-largest runs by attempt count), so every retained attempt now has a matching run in the rollup. total_runs / omitted_runs / omitted_request_count now flow through CompactionResult. Added phase2_heterogeneous_runs_keep_largest_and_stay_coherent (hot keys introduced last in first-seen order, interleaved with many cold single-attempt keys) — it fails on the old selection and passes now, asserting every retained record has a matching run + exact totals.

2. Live-test silent no-op guard. Replaced the weak max_request_count >= 1 assertion with a loud guard: once the storm batch reaches the service (and we only get there after deriving target_region from a region the baseline probe actually contacted), it must induce retries beyond the fault-free baseline. If the region-scoped faults silently fail to match (e.g. a region-name normalization mismatch), the test now fails with a clear message instead of passing green without exercising compaction.

cargo fmt + cargo clippy --all-features --all-targets -- -D warnings clean; all 61 model compaction unit tests pass.

NaluTripician added a commit to NaluTripician/azure-sdk-for-rust that referenced this pull request Jul 6, 2026
Address the Copilot review comments and unblock the documentation-only
CI checks on PR Azure#4684:

- Reword the two "Shipped today (PR Azure#4683)" claims (in the bounded-size
  section and the Q4 row) to "Proposed in PR Azure#4683 (not yet on `main`)".
  `max_request_diagnostics` is added by the unmerged Azure#4683, so this
  stays consistent with the "standalone on `main`" framing and no longer
  implies a reader will find the symbol via the `[opts]` link, which on
  `main` exposes only `max_summary_size_bytes` and `default_verbosity`.
- Convert the five relative source-file reference links to absolute
  GitHub URLs so the link verification check passes.
- Add the doc's technical terms (flatbuffer, materializer(s),
  uncompacted, underspecified) to the cSpell dictionary.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
NaluTripician and others added 3 commits July 6, 2026 13:41
The retry-storm compaction code in the diagnostics capture model introduced `rollup`, `pingpong`, and `uncompacted` in comments/identifiers, which the cspell Analyze job flagged as unknown words. Add them to the cosmos cspell dictionary so `Check spelling (cspell)` passes.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Resolve conflicts from the new azure_data_cosmos_driver_native crate and
in-memory-emulator updates (Azure#4515) landing in main:

- sdk/cosmos/.cspell.json: keep both sides' new ignoreWords
  (activityid, pinning from main; aggregatable, pingpong from storm-safe),
  preserving alphabetical order.
- Cargo.lock: regenerate so it includes the new
  azure_data_cosmos_driver_native crate while retaining the storm-safe
  criterion dev-dependency; verified consistent with cargo metadata --locked.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
NaluTripician and others added 3 commits July 9, 2026 11:16
Resolve conflicts in .cspell.json and azure_data_cosmos_driver Cargo.toml,
and align the capture_engine module with the upstream TransportKind rename
(Gateway20 -> GatewayV2).

- .cspell.json: keep both new ignore words (uncollapsed, uncompacted).
- Cargo.toml: take upstream fault_injection = [] and preview_dtx (rand is now
  non-optional) while keeping the PR's capture_engine feature.
- capture/{recorder,context}.rs: rename TransportKind::Gateway20 references to
  GatewayV2 to match the merged enum definition.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Resolve conflicts after upstream/main changed the default diagnostics
verbosity to Summary and switched to sharing the runtime's configured
DiagnosticsOptions.

- diagnostics_options.rs: keep the new max_request_diagnostics option and
  its tests; adopt upstream's Summary default in docs and the defaults test.
- cosmos_driver.rs: build the diagnostics envelope from the runtime's
  shared options (diagnostics_options_arc) while keeping the
  non-fault_injection unused-var guard.
- capture/model.rs: retain both branches' new diagnostics tests.
- Cargo.lock: reconcile with upstream/main.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@NaluTripician

Copy link
Copy Markdown
Contributor Author

Superseded by #4789. The retry-storm bounding algorithm (max_request_diagnostics cap, run-collapse with exact aggregates, explicit truncation markers, order-robust key-bucket fallback) has been re-implemented over the existing upstream DiagnosticsContext/RequestDiagnostics (no dependency on the prototype capture engine) and is included in #4789 (WS6). Closing this first-pass prototype.

NaluTripician added a commit to NaluTripician/azure-sdk-for-rust that referenced this pull request Jul 16, 2026
Addresses the 13 Copilot reviewer threads on PR Azure#4789:

- Dispatch diagnostics on operation failure via a result-aware
  complete_result seam, and skip the empty-chain Arc clone (clients).
- Thread the handler chain + operation identity into the query and
  change-feed iterators so paged operations emit once per page, and
  populate returned_rows from the page item count.
- Read the operation name from CosmosOperationContext before the
  tracing/logging threshold gate (is_threshold_violated_for) so
  non-point operations use the 3s threshold.
- Rebuild aggregate compaction metadata and re-bound the concatenated
  list in aggregate_sub_operations so request_count() stays exact.
- Include total_request_charge in DiagnosticsContext PartialEq.
- Derive SafeDebug on CosmosOperationContext.
- Count only reserve admissions in the sampling-log failure reserve.
- Omit the misleading active_instance.count metric until client
  lifecycle wiring exists.
- Link/condense the SDK and driver CHANGELOG entries (retarget the
  compaction entry from the closed Azure#4683 to Azure#4789).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Cosmos The azure_cosmos crate

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants